-
Notifications
You must be signed in to change notification settings - Fork 29
Support Py_GIL_DISABLED #206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Replicated with the command: |
Correct, and this will be the case until a new stable ABI is available to target. A PEP is needed in order to create the new stable ABI, and there's some initial discussion about it here: https://discuss.python.org/t/making-pyobject-opaque-in-the-limited-api/77206 |
|
Please rebase now the #205 has been merged. |
b750891 to
41a90cd
Compare
|
@sethmlarson Can you please approve the GitHub Actions workflow below so we can see if the tests pass? |
41a90cd to
fc573cc
Compare
|
Hi all, I was planning to work on this, along with updating all the release automation to support building cp314t wheels (if that's ok with @sethmlarson). IMO it's probably not a good idea to merge this without at least adding a cp314t testing job. |
fdaa76b to
fc573cc
Compare
The double negatives make that line a bit confusing. I believe that you intend to say: IMO it's a good idea to add a cp314t testing job before merging this. Should rebase to pick up the Py3.13 and 3.14 tests landed in: |
Yup, that's what I meant. |
fc573cc to
ebd1aa9
Compare
|
Please add the change in #210 to this pull request. |
Do not set py_limited_api when Py_GIL_DISABLED == 1, since it
will be rejected as shown:
File "/usr/lib/python3.14t/site-packages/setuptools/_distutils/cmd.py", line 135, in ensure_finalized
self.finalize_options()
~~~~~~~~~~~~~~~~~~~~~^^
File "<string>", line 82, in finalize_options
File "/usr/lib/python3.14t/site-packages/setuptools/command/bdist_wheel.py", line 250, in finalize_options
self._validate_py_limited_api()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/usr/lib/python3.14t/site-packages/setuptools/command/bdist_wheel.py", line 285, in _validate_py_limited_api
raise ValueError(
...<4 lines>...
)
ValueError: `py_limited_api='cp314'` not supported. `Py_LIMITED_API` is currently incompatible with `Py_GIL_DISABLED`.See python/cpython#111506.
ebd1aa9 to
2dd9a6c
Compare
|
@ngoldbaum Is this the correct way to handle this situation for free-threading? Wouldn't another option be to disable the limited API altogether if free-threading was enabled? You would know better than I, probably. |
|
We want to be testing free-threading (this PR) before declaring the code to be compatible. My bet is that we will want to be running |
|
@cclauss @ngoldbaum Is there value in getting a 1.2.0.0 release out ahead of adding support for free-threading? We can release a 1.2.0.1 that adds support for free-threading at any point. |
|
Hi, sorry I haven't looked at anything over here yet. I'm hoping to carve some time out today but otherwise I should have time later this week. Please don't feel like cp314t support is necessary to do a release. As you said, that can come in a bugfix release. |
|
@cclauss Is merging this PR confirming that this is the right way to handle this? See: #206 (comment) I was expecting maybe a reference or a link to another project in this same scenario as us. |
|
Current state: We are running tests against We can now add more tests to help us be more confident in free-threading compatibility. As we progress, automated tests will tell us if things break with and without the GIL. nltk/nltk is a project that is in a similar state. |
|
@cclauss in the future I'd appreciate it if you could wait for me to review code. Particularly when I indicate that I want to look at stuff but don't have time. |
|
Yeah my initial thought now that this is merged is that we should maybe... revert it until we get a review from @ngoldbaum? I want to make absolutely sure our build config is correct, because anything we do others are liable to copy, too, if this is not well-trodden ground. |
|
I thought @zmedico's description in the commit message was convincing. Given that this change only affects free-threaded builds (which we do not yet support) I do not see how this change has any effect. |
|
So saying we don't support something doesn't mean that people won't use or try it :) I would actually prefer things keep on breaking until we have a solution we're happy with and are convinced its correct. |
|
I would also prefer backing it out. You could take a look at the PR where we enable cp314t wheel builds in pynacl, as an example: pyca/pynacl#880 It looks like over there the I'd like to do a pass over the wheel-building configuration in this repository to hopefully make it as painless as possible to ship version-specific wheels on the free-threaded build. @cclauss please please give me some time to look at things and work on things. I will be looking at shipping cp314t wheels, it's just not an emergency and it's ok if it takes a little while to get done. Now that you and I can trigger builds it will be much quicker. You can also rely on my experience here instead of trying to figure out everything on your own. I also think @sethmlarson would prefer if we hold off on the free-threaded stuff until after v1.2.0 ships. |
Do not set py_limited_api when Py_GIL_DISABLED == 1, since it will be rejected as shown:
See python/cpython#111506.